MONGOCRYPT-755 Implement StrEncode #928

marksg07 · 2024-12-20T20:09:54Z

No description provided.

src/mc-text-search-str-encode-private.h

src/mc-text-search-str-encode.c

src/mc-text-search-str-encode-private.h

src/mc-text-search-str-encode.c

src/mc-str-encode-string-sets-private.h

src/mc-str-encode-string-sets.c

src/mc-text-search-str-encode.c

kevinAlbs

Nice work. Only substantial comment is to limit string lengths to prevent possible overflows.

kevinAlbs · 2025-01-15T15:00:31Z

test/test-mc-text-search-str-encode.c

+    fprintf(stderr,
+            "Testing nofold suffix/prefix case: str=\"%s\", lb=%u, ub=%u, unfolded_codepoint_len=%u\n",
+            str,
+            lb,
+            ub,
+            unfolded_codepoint_len);


Suggest using the TEST_PRINTF and TEST_STDERR_PRINTF macros to flush stdout/stderr and avoid mixed output in Evergreen logs. The macros were recently introduced in b193dba. Run git merge master to include them.

TEST_STDERR_PRINTF("Testing nofold suffix/prefix case: str=\"%s\", lb=%u, ub=%u, unfolded_codepoint_len=%u\n", str, lb, ub, unfolded_codepoint_len);

Changed to TEST_PRINTF's, thanks for this note.

kevinAlbs · 2025-01-16T16:56:03Z

test/test-mc-text-search-str-encode.c

+#undef MIN
+#define MIN(a, b) (((a) < (b)) ? (a) : (b))


Suggested change

#undef MIN

#define MIN(a, b) (((a) < (b)) ? (a) : (b))

Suggest using BSON_MIN to simplify.

Ah, totally forgot to do this in the test; changed it in the source file due to Erwin's comment earlier. Done.

kevinAlbs · 2025-01-16T16:58:48Z

src/mc-fle2-encryption-placeholder-private.h

@@ -119,6 +119,58 @@ bool mc_FLE2RangeInsertSpec_parse(mc_FLE2RangeInsertSpec_t *out,
                                  bool use_range_v2,
                                  mongocrypt_status_t *status);

+typedef struct {
+    // mlen is the max string length that can be indexed.


Suggested change

// mlen is the max string length that can be indexed.

// mlen is the max string length (in characters, not bytes) that can be indexed.

Added a clarifying comment above.

kevinAlbs · 2025-01-16T20:23:20Z

test/test-mc-text-search-str-encode.c

+            mc_FLE2TextSearchInsertSpec_t spec =
+                {str, byte_len, {{0, 0, 0}, false}, {{lb, ub}, true}, {{0, 0}, false}, false, false};


Suggested change

mc_FLE2TextSearchInsertSpec_t spec =

{str, byte_len, {{0, 0, 0}, false}, {{lb, ub}, true}, {{0, 0}, false}, false, false};

mc_FLE2TextSearchInsertSpec_t spec = {.v = str, .len = byte_len, .suffix = {{lb, ub}, true}};

Suggest using designated initializers and omitting fields that are expected to be zero-initialized to improve readability.

Done. Thanks for the feedback.

kevinAlbs · 2025-01-16T20:30:08Z

test/test-mc-text-search-str-encode.c

+        uint32_t affix_count = 0;
+        uint32_t total_real_affix_count = 0;
+        while (mc_affix_set_iter_next(&it, &affix, &affix_len, &affix_count)) {
+            // Since all substrings are just views on the base string, we can use pointer math to find our start and


Suggested change

// Since all substrings are just views on the base string, we can use pointer math to find our start and

// Since all substrings are just views on the base string, we can use pointer math to find our start and end

kevinAlbs · 2025-01-16T21:05:23Z

test/test-mc-text-search-str-encode.c

+    ASSERT(sets->exact.len == byte_len);
+    ASSERT(0 == memcmp(sets->exact.data, str, byte_len));
+
+    if (unfolded_codepoint_len > mlen || lb > max_padded_len) {


Suggested change

if (unfolded_codepoint_len > mlen || lb > max_padded_len) {

if (lb > max_padded_len) {

Redundant with above check.

Good catch.

kevinAlbs · 2025-01-17T14:02:52Z

src/mc-str-encode-string-sets.c

+    }
+    set->start_indices[idx] = base_start_idx;
+    set->end_indices[idx] = base_end_idx;
+    set->substring_counts[idx] = 1;


Consider storing and incrementing the current set size in mc_affix_set_t, rather than requiring callers to track the index:

set->start_indices[set->cur_idx] = base_start_idx; set->end_indices[set->cur_idx] = base_end_idx; set->substring_counts[set->cur_idx] = 1; set->cur_idx++;

That may help avoid exposing implementation details of mc_affix_set_t to the caller.

Changed, thanks for the suggestion.

kevinAlbs · 2025-01-17T14:05:45Z

src/mc-str-encode-string-sets.c

+    it->cur_idx = 0;
+}
+
+bool mc_affix_set_iter_next(mc_affix_set_iter_t *it, const char **str, uint32_t *len, uint32_t *count) {


Suggested change

bool mc_affix_set_iter_next(mc_affix_set_iter_t *it, const char **str, uint32_t *len, uint32_t *count) {

bool mc_affix_set_iter_next(mc_affix_set_iter_t *it, const char **str, uint32_t *byte_len, uint32_t *count) {

To clarify output is byte length, not character length.

kevinAlbs · 2025-01-17T14:06:58Z

src/mc-str-encode-string-sets.c

+// Linked list node in the hashset.
+typedef struct _mc_substring_set_node_t {
+    uint32_t start_offset;
+    uint32_t len;


Suggested change

uint32_t len;

uint32_t byte_len;

kevinAlbs · 2025-01-17T14:57:43Z

src/mc-str-encode-string-sets.c

+mc_utf8_string_with_bad_char_t *mc_utf8_string_with_bad_char_from_buffer(const char *buf, uint32_t len) {
+    BSON_ASSERT_PARAM(buf);
+    mc_utf8_string_with_bad_char_t *ret = bson_malloc0(sizeof(mc_utf8_string_with_bad_char_t));
+    _mongocrypt_buffer_init_size(&ret->buf, len + 1);


I expect this could overflow if len is UINT32_MAX. Similarly, the CBC length calculations may overflow when adding 15.

I suggest rejecting too-long strings in mc_text_search_str_encode, and using a limit much smaller than UINT32_MAX. If the limit is near UINT32_MAX, these operations may be prohibitively slow to be useful and could risk a denial-of-service attack. Consider using 16MiB (16777216 bytes) to match the maximum insert size of a BSON document (maxBsonObjectSize from the hello command reply).

I was originally thinking that we would perform this check at a higher level, but I think now I agree with you that it makes sense to do the checks here, nearer to where the actual algorithm is taking place. One thing to note is that for the substring case, even getting close to 16MiB is going to be way too big. Figuring out the max values for all the parameters that go in is still TBD for later in the project, so I'll put the limit at 16MiB for now and we can think more about this later.

marksg07 added 10 commits December 20, 2024 20:09

MONGOCRYPT-755 Implement StrEncode

70e2ef4

Comments + cleanup

fe6f93b

more comments

c8678c8

fix

5215b80

fix ff

e5e8c58

fix

92bfeb0

f

ceacd48

windows

cbd420d

ll

54f6815

lld

481f378

marksg07 requested review from kevinAlbs, erwee and markbenvenuto December 31, 2024 20:07

marksg07 marked this pull request as ready for review December 31, 2024 20:07

Merge branch 'master' into marksg07/mongocrypt-755

85a12ba

erwee requested changes Jan 2, 2025

View reviewed changes

marksg07 marked this pull request as draft January 6, 2025 16:02

marksg07 removed request for kevinAlbs and markbenvenuto January 6, 2025 16:02

marksg07 added 3 commits January 6, 2025 21:23

unicode

723427d

comment

0286858

comments

b0c023f

marksg07 requested a review from erwee January 6, 2025 21:36

marksg07 added 2 commits January 6, 2025 21:43

const

cb6bcf2

windows

10792c2

marksg07 marked this pull request as ready for review January 7, 2025 16:20

marksg07 requested review from kevinAlbs and markbenvenuto January 7, 2025 16:20

erwee requested changes Jan 9, 2025

View reviewed changes

src/mc-text-search-str-encode.c Outdated Show resolved Hide resolved

marksg07 added 4 commits January 10, 2025 22:25

Hashset

4bcba8a

PR fixes

3e0301e

fix bug

dad5688

a

48f80c1

marksg07 requested a review from erwee January 13, 2025 21:18

marksg07 added 2 commits January 13, 2025 21:59

more leaks

59e5944

Merge branch 'master' into marksg07/mongocrypt-755

67b5d07

erwee requested changes Jan 15, 2025

View reviewed changes

Fixes

d8f11cb

marksg07 requested a review from erwee January 15, 2025 21:59

erwee approved these changes Jan 16, 2025

View reviewed changes

kevinAlbs reviewed Jan 17, 2025

View reviewed changes

marksg07 added 2 commits January 21, 2025 17:56

Merge branch 'master' into marksg07/mongocrypt-755

95786df

pr

b75e949

marksg07 requested a review from kevinAlbs January 21, 2025 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MONGOCRYPT-755 Implement StrEncode #928

MONGOCRYPT-755 Implement StrEncode #928

marksg07 commented Dec 20, 2024

kevinAlbs left a comment

kevinAlbs Jan 15, 2025

marksg07 Jan 21, 2025

kevinAlbs Jan 16, 2025

marksg07 Jan 21, 2025

kevinAlbs Jan 16, 2025

marksg07 Jan 21, 2025

kevinAlbs Jan 16, 2025

marksg07 Jan 21, 2025

kevinAlbs Jan 16, 2025

marksg07 Jan 21, 2025

kevinAlbs Jan 16, 2025

marksg07 Jan 21, 2025

kevinAlbs Jan 17, 2025

marksg07 Jan 21, 2025

kevinAlbs Jan 17, 2025

marksg07 Jan 21, 2025

kevinAlbs Jan 17, 2025

marksg07 Jan 21, 2025

kevinAlbs Jan 17, 2025

marksg07 Jan 21, 2025

	// mlen is the max string length that can be indexed.
	// mlen is the max string length (in characters, not bytes) that can be indexed.

		mc_FLE2TextSearchInsertSpec_t spec =
		{str, byte_len, {{0, 0, 0}, false}, {{lb, ub}, true}, {{0, 0}, false}, false, false};

	mc_FLE2TextSearchInsertSpec_t spec =
	{str, byte_len, {{0, 0, 0}, false}, {{lb, ub}, true}, {{0, 0}, false}, false, false};
	mc_FLE2TextSearchInsertSpec_t spec = {.v = str, .len = byte_len, .suffix = {{lb, ub}, true}};

	// Since all substrings are just views on the base string, we can use pointer math to find our start and
	// Since all substrings are just views on the base string, we can use pointer math to find our start and end

	if (unfolded_codepoint_len > mlen \|\| lb > max_padded_len) {
	if (lb > max_padded_len) {

	bool mc_affix_set_iter_next(mc_affix_set_iter_t it, const char str, uint32_t len, uint32_t *count) {
	bool mc_affix_set_iter_next(mc_affix_set_iter_t it, const char str, uint32_t byte_len, uint32_t *count) {

MONGOCRYPT-755 Implement StrEncode #928

Are you sure you want to change the base?

MONGOCRYPT-755 Implement StrEncode #928

Conversation

marksg07 commented Dec 20, 2024

kevinAlbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment